-
Notifications
You must be signed in to change notification settings - Fork 504
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Checks for auth token on app refresh #2793
Checks for auth token on app refresh #2793
Conversation
317c526
to
7b8a9c0
Compare
Thanks for the PR, @vindex10 . I'll take a look and circle back soon. |
Changes look good to me. I'll let @kevgliss take a look at them before merging them. |
@vindex10 Could you explain a little more about the behavior you are seeing? Locally, I can't replicate what you describe (a page refresh does not force login if the user has a valid token). If the app state gets wiped (e.g. by a refresh), we explicitly test for an existing token here and use if it's available:
Local storage should not be wiped out by a refresh (a user needs to do a hard refresh or otherwise delete this data). |
OK, thanks! indeed it works. For dev, I had to:
For prod I had to change docker-compose.yaml:
And basic auth is enabled by default in the backend? So should this become a part of? |
upd: disregard this. it was a wrong guess, and the message above works now for both dev and compose run. Hmm. It actually didn't work with docker compose. And I have a suspect. In the setup.py, it looks like the Line 147 in e52af85
and build_static that used to build the frontend before, doesn't do it anymore: Line 306 in e52af85
but it actually handled env vars before. |
sorry for the noise :) in summary:
I'm trying to understand now, which PR to open :) I see the options:
What confuses me is the slight inconsistency between: api.js:
where default auth is assumed to be "basic auth" router/index.js:
here empty authentication slug falls back to custom auth (which seems to be a feature, not a bug) backend config.py: dispatch/src/dispatch/config.py Line 100 in 17e1a64
@kevgliss look forward to your feedback ) |
I think your right when you point out the inconsistency here:
I think the change should align the default to basic-auth similar to what we do here:
So, in summary I think we should define:
here: I think this is option # 3 that you mentioned. |
@kevgliss done. I checked, at least it worked when I ran locally :) |
LGTM, thanks for the contribution! |
* make basic auth provider default in vue
Hi, All!
I noticed that, at least, in case of basic auth, the token stored in the storage is not checked for. Therefore, upon refresh user needs to login again.
I suggest some potential change that fixes this. I'm a newcomer to dispatch, so I expect I will learn something new soon :)
Look forward to your feedback!
Cheers,
Victor